Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Closes #7075: Plugin - Accommodate new oneclick exclusions UI for delayjs #7090

Open
wants to merge 43 commits into
base: feature/3.18
Choose a base branch
from
Open

Closes #7075: Plugin - Accommodate new oneclick exclusions UI for delayjs #7090

wants to merge 43 commits into from

Conversation

jeawhanlee
Copy link
Contributor

Description

Fixes #7075
Explain how this code impacts users.

Type of change

  • Enhancement (non-breaking change which improves an existing functionality).

Detailed scenario

N/A

Technical description

Documentation

This code adds a parent category to the scripts and adapts compatibility with the new delayjs backend exclusion list structure. it also adds some enhancement to the UI/UX.

New dependencies

N/A

Risks

N/A

Mandatory Checklist

Code validation

  • I validated all the Acceptance Criteria. If possible, provide screenshots or videos.
  • I triggered all changed lines of code at least once without new errors/warnings/notices.
  • I implemented built-in tests to cover the new/changed code.

Code style

  • I wrote a self-explanatory code about what it does.
  • I did not introduce unnecessary complexity.

Additional Checks

  • In the case of complex code, I wrote comments to explain it.
  • I added error handling logic when using functions that could throw errors (HTTP/API request, filesystem, etc.)

@jeawhanlee jeawhanlee added the epics 🔥 For large tasks or features, broken into smaller issues. label Nov 5, 2024
@jeawhanlee jeawhanlee added this to the 3.18 milestone Nov 5, 2024
@jeawhanlee jeawhanlee self-assigned this Nov 5, 2024
@jeawhanlee jeawhanlee linked an issue Nov 5, 2024 that may be closed by this pull request
Plugin - Accommodate new oneclick exclusions UI for delayjs #7075
Open
@codacy-production Codacy Production
Copy link

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
Report missing for c2e65f01 ✅ 85.58% (target: 50.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (c2e65f0) Report Missing Report Missing Report Missing
Head commit (64d522b) 38186 16801 44.00%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#7090) 104 89 85.58%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

Footnotes

  1. Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩

@jeawhanlee
Copy link
Contributor Author

jeawhanlee commented Nov 5, 2024 •
edited
Loading

Old UI

Screenshot 2024-11-05 at 14 31 06

New UI Changes

Screenshot 2024-11-05 at 14 28 14

@jeawhanlee
Copy link
Contributor Author

jeawhanlee commented Nov 5, 2024 •
edited
Loading

Automated testing passes with the new data structure, awaiting backend implementation to be sure it truly passes for manual testing.

@jeawhanlee
Copy link
Contributor Author

Icons implementation pending.
CC @piotrbak

@jeawhanlee jeawhanlee marked this pull request as ready for review November 5, 2024 13:42
@jeawhanlee jeawhanlee requested a review from a team November 8, 2024 09:47
@piotrbak
Copy link
Contributor

piotrbak commented Nov 8, 2024

@jeawhanlee I don't have access to this Figma. Checking with @DahmaniAdame if he has

@DahmaniAdame
Copy link
Contributor

@jeawhanlee I don't have access to this Figma. Checking with @DahmaniAdame if he has

@piotrbak I don't. Ben was the only one with access to it.

@jeawhanlee
Copy link
Contributor Author

@wp-media/qa-team For testing you can replace this line with the last endpoint in the comment here and update lists from tools section in WPR dashboard.

@Mai-Saad
Copy link
Contributor

Mai-Saad commented Dec 6, 2024 •
edited
Loading

@jeawhanlee Thanks for the update , remain updating the 3rd party icons
Screenshot from 2024-12-06 12-19-13
Note:

  • after update from 3.17.3.1 , we need to manually update dynamic lists to see scripts, this will probably be handled here Update dynamic lists deploy workflow with the new endpoint #7113 (we may need to update the snippet which creates zip for testing)
  • When rollback from PR while some items are checked , we will have this warning and analytics will be empty @jeawhanlee can you please check
    [06-Dec-2024 10:34:22 UTC] PHP Warning: Attempt to read property "title" on array in /var/www/new.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Optimization/DelayJS/Admin/SiteList.php on line 215
    Screenshot from 2024-12-06 12-39-16

@DahmaniAdame
Copy link
Contributor

Icons can be found on this thread - https://wp-media.slack.com/archives/C06CQPWEJSK/p1733497507056019?thread_ts=1733395114.523529&cid=C06CQPWEJSK

@jeawhanlee jeawhanlee requested a review from a team December 10, 2024 09:56
wordpressfan
wordpressfan requested changes Dec 10, 2024
View reviewed changes
Copy link
Contributor

@wordpressfan wordpressfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Icons image sizes are not exactly the same which is causing not good UI:

image

Can we make sure that the size is 16X16 so it has no UI problems?
also I can see that not all of them are images but some of them are dashicons so make sure that their width and height are exactly the same too and if it's not working properly with dashicons convert all of them to be images.

what do u think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wordpressfan wordpressfan wordpressfan approved these changes

@Miraeld Miraeld Awaiting requested review from Miraeld

Assignees

@jeawhanlee jeawhanlee

Labels
epics 🔥 For large tasks or features, broken into smaller issues.
Projects
None yet
Milestone
3.18
Development

Successfully merging this pull request may close these issues.

Plugin - Accommodate new oneclick exclusions UI for delayjs
6 participants
@jeawhanlee @piotrbak @DahmaniAdame @Mai-Saad @Miraeld @wordpressfan